Make sure to reject mappings with type _doc when include_type_name is false. by jtibshirani · Pull Request #38270 · elastic/elasticsearch

jtibshirani · 2019-02-03T02:02:32Z

CreateIndexRequest#source(Map<String, Object>, ... ), which is used when deserializing index creation requests, accidentally accepts mappings that are nested twice under the type key (as described in the bug report #38266).

This in turn causes us to be too lenient in parsing typeless mappings. In particular, we accept the following index creation request, even though it should not contain the type key _doc:

PUT index?include_type_name=false
{
  "mappings": {
    "_doc": {
      "properties": { ... }
    }
}

There is a similar issue for both 'put templates' and 'put mappings' requests as well.

This PR makes the minimal changes to detect and reject these typed mappings in requests. It does not address #38266 generally, or attempt a larger refactor around types in these server-side requests, as I think this should be done at a later time.

…lude_type_name is false.

elasticmachine · 2019-02-03T02:02:34Z

Pinging @elastic/es-search

elasticmachine · 2019-02-03T02:02:35Z

Pinging @elastic/es-core-features

jpountz

I left some minor comments but in general it looks good to me. Thanks for extracting the map manipulation code to functions that can be unit-tested.

jpountz · 2019-02-04T09:56:25Z

rest-api-spec/src/main/resources/rest-api-spec/test/indices.create/10_basic.yml

+      version: " - 6.99.99"
+      reason: include_type_name defaults to true before 7.0
+  - do:
+      catch: /illegal_argument_exception/


can we match a more specific string in the error message?

jpountz · 2019-02-04T09:56:36Z

rest-api-spec/src/main/resources/rest-api-spec/test/indices.put_mapping/10_basic.yml

+        index: test_index
+
+  - do:
+      catch: /illegal_argument_exception/


can we match a more specific string in the error message?

jpountz · 2019-02-04T09:56:42Z

rest-api-spec/src/main/resources/rest-api-spec/test/indices.put_template/10_basic.yml

+      reason: include_type_name defaults to true before 7.0
+
+  - do:
+      catch: /illegal_argument_exception/


can we match a more specific string in the error message?

jpountz · 2019-02-04T10:00:56Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestCreateIndexAction.java

+
+
+    static Map<String, Object> prepareMappings(Map<String, Object> source, boolean includeTypeName) {
+        if (includeTypeName || !source.containsKey("mappings")) {


use == false for consistency with the rest of the codebase?

Will do, this is proving a hard habit to break!

jpountz · 2019-02-04T10:03:21Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestCreateIndexAction.java

+        Map<String, Object> newSource = new HashMap<>(source);
+
+        @SuppressWarnings("unchecked")
+        Map<String, Object> mappings = (Map<String, Object>) source.get("mappings");


This cast is unsafe and would result in a confusing error message if a user created an index with something like below? Should we have an instanceof Map check?

PUT test { "mappings": "foo" }

Thanks, will fix this.

Just a note that I made this method more robust, but after testing I noticed the same confusing error will occur even without these changes. This is because of another unchecked cast: https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/action/admin/indices/create/CreateIndexRequest.java#L381

Thanks for looking

jtibshirani · 2019-02-04T18:07:54Z

Thank you @jpountz for reviewing, it is ready for another look.

jakelandis

LGTM

…eless-requests

jpountz · 2019-02-05T12:57:47Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestCreateIndexAction.java

+        Map<String, Object> newSource = new HashMap<>(source);
+
+        @SuppressWarnings("unchecked")
+        Map<String, Object> mappings = (Map<String, Object>) source.get("mappings");


Thanks for looking

…eless-requests

* master: (23 commits) Lift retention lease expiration to index shard (elastic#38380) Make Ccr recovery file chunk size configurable (elastic#38370) Prevent CCR recovery from missing documents (elastic#38237) re-enables awaitsfixed datemath tests (elastic#38376) Types removal fix FullClusterRestartIT warnings (elastic#38445) Make sure to reject mappings with type _doc when include_type_name is false. (elastic#38270) Updates the grok patterns to be consistent with logstash (elastic#27181) Ignore type-removal warnings in XPackRestTestHelper (elastic#38431) testHlrcFromXContent() should respect assertToXContentEquivalence() (elastic#38232) add basic REST test for geohash_grid (elastic#37996) Remove DiscoveryPlugin#getDiscoveryTypes (elastic#38414) Fix the clock resolution to millis in GetWatchResponseTests (elastic#38405) Throw AssertionError when no master (elastic#38432) `if_seq_no` and `if_primary_term` parameters aren't wired correctly in REST Client's CRUD API (elastic#38411) Enable CronEvalToolTest.testEnsureDateIsShownInRootLocale (elastic#38394) Fix failures in BulkProcessorIT#testGlobalParametersAndBulkProcessor. (elastic#38129) SQL: Implement CURRENT_DATE (elastic#38175) Mute testReadRequestsReturnLatestMappingVersion (elastic#38438) [ML] Report index unavailable instead of waiting for lazy node (elastic#38423) Update Rollup Caps to allow unknown fields (elastic#38339) ...

elastic/elasticsearch#38270 made current master (7.0.0) reject mappings with type `_doc` unless `?include_type_name=true` is passed during index creation. Workaround the rejection for now by explicitly setting `include_type_name` to `true` in every create_index operation.

elastic/elasticsearch#38270 made current master (7.0.0) reject mappings with type `_doc` unless `?include_type_name=true` is passed during index creation. Workaround the rejection for now by explicitly setting `include_type_name` to `true` in every create_index operation. Relates: #64

jtibshirani added 2 commits February 2, 2019 17:34

Pull MetaDataMappingService#isMappingSourceTyped into MapperService.

dddf95e

Make sure to reject typed index creation + template requests when inc…

6ce1eb0

…lude_type_name is false.

jtibshirani added >bug :Search Foundations/Mapping Index mappings, including merging and defining field types :Data Management/Indices APIs DO NOT USE. Use ":Distributed/Indices APIs" or ":StorageEngine/Templates" instead. v7.0.0 v6.7.0 labels Feb 3, 2019

jtibshirani requested review from jakelandis and jpountz February 3, 2019 02:02

jtibshirani added 4 commits February 3, 2019 13:49

Unify some mappings logic between index and template creation.

765979b

Fix the same issue in put mapping requests.

aff2299

Make sure test set-up code uses typeless APIs.

93ec33f

Fix some REST tests.

82066cf

jtibshirani force-pushed the dummy-type-in-typeless-requests branch from 2bbf871 to 82066cf Compare February 3, 2019 22:52

jtibshirani added 3 commits February 3, 2019 16:06

Fix more integration tests.

9ac6ba7

Fix the SQL plugin tests.

9cc6ed6

Fix the x-pack multi node tests.

8175962

jpountz suggested changes Feb 4, 2019

View reviewed changes

jtibshirani added 3 commits February 4, 2019 09:43

Check for the correct error message in REST yml tests.

ecbd76c

Make sure to use 'condition == false'.

06b202e

Handle malformed mappings in RestCreateIndexAction#prepareMappings.

a24123d

jakelandis approved these changes Feb 4, 2019

View reviewed changes

jtibshirani added 2 commits February 4, 2019 16:26

Merge remote-tracking branch 'upstream/master' into dummy-type-in-typ…

98a9d9b

…eless-requests

Apply the same fix for rollover requests.

e5af064

jpountz approved these changes Feb 5, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into dummy-type-in-typ…

b312148

…eless-requests

jtibshirani added the >breaking label Feb 5, 2019

jtibshirani merged commit 3ce7d2c into elastic:master Feb 5, 2019

jtibshirani deleted the dummy-type-in-typeless-requests branch February 5, 2019 18:52

jtibshirani added the backport pending label Feb 5, 2019

jtibshirani mentioned this pull request Feb 6, 2019

Make sure to reject mappings with type _doc when include_type_name is false. #38484

Merged

dliappis mentioned this pull request Feb 6, 2019

Allow _doc type in master elastic/rally-tracks#64

Merged

tsullivan mentioned this pull request Feb 6, 2019

[Task Manager] remove [DOC_TYPE] from index template body elastic/kibana#30261

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

dliappis mentioned this pull request Feb 7, 2019

indices.create doesn't support include_type_name request parameter elastic/elasticsearch-py#901

Closed

jtibshirani removed the backport pending label Feb 9, 2019

danielmitterdorfer removed the :Data Management/Indices APIs DO NOT USE. Use ":Distributed/Indices APIs" or ":StorageEngine/Templates" instead. label Feb 11, 2019

jtibshirani removed the >breaking label Feb 22, 2019

jtibshirani mentioned this pull request Apr 12, 2021

[Rest Compatible Api] include_type_name parameter #70966

Merged



		static Map<String, Object> prepareMappings(Map<String, Object> source, boolean includeTypeName) {
		if (includeTypeName \|\| !source.containsKey("mappings")) {

Conversation

jtibshirani commented Feb 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Feb 3, 2019

Uh oh!

elasticmachine commented Feb 3, 2019

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtibshirani Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtibshirani commented Feb 4, 2019

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jtibshirani commented Feb 3, 2019 •

edited

Loading

jtibshirani Feb 4, 2019 •

edited

Loading